Style: narrow scope of rpcport_arg since it is only used inside the if-block. (In this case the outer scope ends it anyway, this is just a general rule of thumb).

Naming: rpcport_port -> rpcport_int since the former is a bit repetitive and the latter at least describes its purpose somewhat, despite being a bit Hungarian.

Good suggestions. I've implemented these changes and added a small comment to increase readability (since operator bool of std::optional is being used).

In caa5242 "cli: Sanitize ports in rpcconnect and rpcport"

This will fail if you just have a regtest node running independent of the test:

2024-04-22T22:30:22.751000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/util.py", line 122, in assert_raises_process_error
    fun(*args, **kwds)
  File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/test_node.py", line 821, in __call__
    return self.cli.send_cli(self.command, *args, **kwargs)
  File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/test_node.py", line 886, in send_cli
    raise subprocess.CalledProcessError(returncode, self.binary, output=cli_stderr)
subprocess.CalledProcessError: Command '/home/ava/bitcoin/bitcoin/29521/src/bitcoin-cli' returned non-zero exit status 1.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "/home/ava/bitcoin/bitcoin/29521/test/functional/interface_bitcoin_cli.py", line 136, in run_test
    assert_raises_process_error(1, "Could not connect to the server 127.0.0.1:18443", self.nodes[0].cli("-rpcconnect=127.0.0.1").echo)
  File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/util.py", line 127, in assert_raises_process_error
    raise AssertionError("Expected substring not found:" + e.output)
AssertionError: Expected substring not found:error: Authorization failed: Incorrect rpcuser or rpcpassword

Thanks for taking a look and trying it out @achow101. Much appreciated.

Good point. Yes, the test would fail if an independent node is running on 18443.

Although not ideal, do you think this would be reasonable since a warning is provided when bitcoind is running independently? (i.e. WARNING! There is already a bitcoind process running on this system. Tests may fail unexpectedly due to resource contention!)

Alternatively, the assert statement could be removed, but that seems a bit lacking, since it seems good to check for default port usage (even if the current check is limited to the default regtest port).

Could also look into refactoring a bit, but that might lead to touching a bit more code than desirable.

Thoughts?

I'd say it should be avoided to use test outside of the allowed range assigned to this test case. Each test is assigned 12, or so, p2p and rpc ports. I think this test can be dropped, because if the default behavior changed, pretty much every developer would notice quickly anyway?

Yes, that sounds reasonable. Overall, port error checking and associated functional test cases introduce an improvement. Removed the "18443" test and pushed.

I added this line and it seemed to pass

assert_raises_process_error(1, 'timeout on transient error: Could not connect to the server 127.0.0.1:::', self.nodes[0].cli("-rpcconnect=127.0.0.1::").echo)

looks like SplitHostPort does not throw an exception if there are two colons and no port provided

I am unsure if that is needed logic in SplitHostPort

I'm seeing the same error when I do
self.nodes[0].cli("-rpcconnect=127.0.0.256")
probably because 255 is the max value

Thanks @kevkevinpal. Good example of why review is valued. Your findings nudged me to add some assert test statements (which otherwise wouldn't be present) to help cover cases involving IPv6 hosts.

We're experiencing the result of the scope of SplitHostPort(). SplitHostPort() extracts the host and port. Although the port is validated, host is not (and the function declaration comment header mentions validating the port, but doesn't mention validating the host).

SplitHostPort() is treating "127.0.0.1::" as an IPv6 host since "::" is used for IPv6 address shortening. SplitHostPort() detects no port as part of the input string, which would be why Could not connect to the server 127.0.0.1:::18443 is the error provided for src/bitcoin-cli -rpcconnect=127.0.0.1:: getblockcount (the ":18443" is tacked on the end because it's the default port for regtest).

An enhancement would be to perform a validation of the host received from SplitHostPort() before bitcoin-cli attempts to contact the host. I'm thinking we would need to first define what is "valid" and what is "invalid" for use with bitcoin-cli. The gist of this kind of checking seems to be present in CNetAddr. In general, I would think bitcoin-cli shouldn't be overly restrictive in what it lets the user attempt to use as a host.

For now, it might make the most sense to leave the enhancement of host validation to a new PR since this PR is focused on port error detection.

(IMO it's slightly cleaner to split the test into --ipv4 and --ipv6 variants similar to rpc_bind.py, that way it becomes explicit whether that part of the test coverage is being skipped or not. This being more of a parsing-test, I guess it is not on the critical end of the spectrum. Also it's just a minor part of interface_bitcoin_cli.py so I completely understand the resistance to breaking it out).

Thanks @cbergqvist. Good observation.

Looks like rpc_bind.py uses --ipv4 and --ipv6 arguments to run the test exclusively with ipv4 or ipv6 (not both), which at first glance doesn't seem to be desirable for interface_bitcoin_cli.py (dependence on network type is a somewhat small part of the test). I could see there being value in refactoring interface_bitcoin_cli.py to include wider coverage of bitcoin-cli with ipv4 and ipv6, but I'm thinking this sort of refactor would be a good follow-up PR rather than included in the scope of this PR.

In the interim, I did push a minor update to add a log message stating when ipv6 testing is being skipped (and also opportunistically rebased).

if you treat 0 as invalid, there is no need to also have nullopt be invalid as well, no?

This should also fix the false-positive compiler warning?

An alternative would be to suppress the false-positive warning in the CI task config.

ToIntegral itself returns a std::optional unfortunately so the suggested change won't compile as is Super-weird that this error popped up now.

I placed the ) in the wrong place, I think. However, value_or exists and should be compile-able. See https://en.cppreference.com/w/cpp/utility/optional/value_or

Yeah, that new version should work. Another possible variant.. but no way to know if the compiler becomes happier:

no way to know if the compiler becomes happier:

If it compiles locally, the code should be fine. If the CI warning persists, it should be disabled in the CI task config. (you can test by pushing, or running locally)

Thanks all for the suggestions. Pushed an update.
Since locally I have x86_64 (and not arm), I tried configuring/compiling with --enable-werror added to ./configure (to minimize excess CI churn). Interestingly enough, this didn't result in the same warning as seen on arm CI.

No luck with the arm CI again. Will try playing with the specific stage locally more (https://github.com/bitcoin/bitcoin/tree/master/ci).

It is a false positive, so it needs to be disabled, like in the other CI tasks.

Fixed in #30144